Fix two latent HWIntrinsic miscompiles in gentree.cpp by tannergooding · Pull Request #130832 · dotnet/runtime

tannergooding · 2026-07-16T01:46:55Z

Two latent correctness fixes found while auditing gentree.cpp, each with a regression test.

Fix GetOperForHWIntrinsicId testing the isScalar pointer instead of *isScalar (#130830)

GetOperForHWIntrinsicId refines a GT_SUB into a GT_NEG when the constant op1 is a scalar zero, but it guarded on the isScalar out-param pointer (always non-null) rather than the dereferenced value. This let a packed subtract whose constant has a zero low lane but is not all-zero (e.g. Vector128.Create(0, 1, 2, 3) - x) be reported as a negate. fgOptimizeHWIntrinsic's (-v1) + v2 => v2 - v1 transform then drops the constant entirely.

Fix stray block clobbering needsFixup in gtNewSimdMinMaxNode (#130831)

A stray unconditional block in the min branch of the floating constant fast path overwrote needsFixup for all four cases, making the preceding if/else dead. needsFixup signals that a signed-zero constant needs the AVX512 fixup so min(+0, -0) keeps the correct sign of zero; with it wrongly forced false, Min/MinNumber against a signed-zero constant miscompiles. The min branch now mirrors the already-correct max branch.

Existing coverage in JitBlue/Runtime_98068 exercises Min/MinNumber const-folding but always pairs an operand with NaN, so the finite opposite-signed-zero case was never tested.

Both are pre-existing on main, found by inspection. Tests added under JitBlue/Runtime_130830 and JitBlue/Runtime_130831.

CC. @dotnet/jit-contrib

Note

This PR description was authored with the help of GitHub Copilot.

The scalar-only IsScalarZero refinement guarded on the isScalar pointer (always non-null) rather than *isScalar, so a packed SUB-mapped intrinsic with a constant op1 whose low element is zero -- but which is not all-zero, e.g. Vector128.Create(0, 5, 6, 7) - x -- was mis-mapped to GT_NEG. morph consumes the packed effective oper and drops the real constant, a latent miscompile. Dereference the flag so the refinement only applies to scalar ops. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

The min-branch signed-zero fast path computed eedsFixup per isNumber/isScalar, then an unconditional trailing block overwrote it with IsVectorNegativeZero for all four cases -- so scalar and non-negative-zero vector cases got the wrong value. When it wrongly lands alse, the const-fold fast path is taken without the required AVX512 fixup, a latent miscompile of Min with a signed-zero constant. Mirror the correct max branch: the non-scalar non-number case uses IsVectorNegativeZero and the stray block is removed. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Covers packed integer `(cns - v1) + v2` where the constant's low lane is zero, which was previously miscompiled by dropping the constant. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Covers Min/MinNumber with a signed-zero constant, the finite opposite-signed-zero case that Runtime_98068 never exercised. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

azure-pipelines · 2026-07-16T01:47:37Z

Azure Pipelines: Successfully started running 5 pipeline(s). 10 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run.

dotnet-policy-service · 2026-07-16T01:48:13Z

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch
See info in area-owners.md if you want to be subscribed.

Copilot

Pull request overview

This PR fixes two latent correctness issues in CoreCLR JIT’s gentree.cpp HWIntrinsic handling and adds targeted JitBlue regression coverage to prevent regressions.

Changes:

Fix GenTreeHWIntrinsic::GetOperForHWIntrinsicId(..., getEffectiveOp: true) to test *isScalar (not the out-param pointer) before treating certain SUB patterns as NEG.
Fix gtNewSimdMinMaxNode (xarch floating const-fold fast path) so needsFixup is computed correctly for signed-zero Min/MinNumber scenarios.
Add new JitBlue regression tests for both miscompiles.

Show a summary per file

File	Description
src/coreclr/jit/gentree.cpp	Correctness fixes for HWIntrinsic effective-op classification and signed-zero fixup gating in SIMD min/max constant folding.
src/tests/JIT/Regression/JitBlue/Runtime_130830/Runtime_130830.cs	Regression test ensuring packed integer subtract isn’t misreported as negate when only the low lane is zero.
src/tests/JIT/Regression/JitBlue/Runtime_130831/Runtime_130831.cs	Regression test intended to validate signed-zero behavior for `Min`/`MinNumber` with constant operands.

Copilot's findings

Files reviewed: 3/3 changed files
Comments generated: 1

Double/Single.Equals treat -0.0 and +0.0 as equal, so the original Assert.Equal assertions did not observe a wrong-signed-zero result. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot

Copilot's findings

Files reviewed: 3/3 changed files
Comments generated: 2

The merged JitBlue tests need an explicit Compile entry to be built and run in CI; without it Runtime_130830 and Runtime_130831 were not exercised. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot

Copilot's findings

Files reviewed: 4/4 changed files
Comments generated: 0 new

tannergooding · 2026-07-16T02:43:48Z

CC. @dotnet/jit-contrib, @EgorBo. Two simple bug fixes with regression tests

`Runtime_129288.cs` was added in #129348 (`JIT: don't claim rotates set ZF on xarch`) as a merged-style xunit test (`[Fact] public static int TestEntryPoint()`), but it was never referenced by any `Regression_*.csproj`. Since the SDK-style CLR test projects set `EnableDefaultItems=false`, an unreferenced `.cs` is silently never built or run -- so CI has never exercised this test. This is the same class of bug fixed in #130832. Add it to `Regression_ro_2.csproj` (an `Optimize=True` bucket, correct for this lowering/codegen correctness test), inserted in numeric order alongside its neighbors. I also audited every other `.cs` under `src/tests/JIT/Regression/` for the same issue. The only genuine orphan was `Runtime_129288`. Six other unreferenced `.cs` files are intentionally uncompiled reference sources paired with a hand-written/generated `.il` + `.ilproj` (`Runtime_70259`, `Runtime_70607`, `Runtime_73615`, `Runtime_80731`, `Runtime_40607`, `DevDiv_754566`) and were left as-is. > [!NOTE] > This PR description and the change were generated with the assistance of GitHub Copilot. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

github-actions · 2026-07-19T13:02:46Z

Workflow state for the Holistic Review Orchestrator.

{
  "version": 5,
  "last_dispatched_commit": "45c9dfe3b26431e9282bbbb6078c7973d0db59c2",
  "last_dispatched_base_ref": "main",
  "last_dispatched_base_sha": "49110af56a95223e88f9e540503c0f8a16e64e15",
  "last_reviewed_commit": "45c9dfe3b26431e9282bbbb6078c7973d0db59c2",
  "last_reviewed_base_ref": "main",
  "last_reviewed_base_sha": "49110af56a95223e88f9e540503c0f8a16e64e15",
  "last_recorded_worker_run_id": "29688130758",
  "review_attempt_commit": "",
  "review_attempt_base_ref": "",
  "review_attempt_count": 0,
  "max_review_attempts": 5,
  "review_history_format": "holistic-review-disclosure-v1",
  "review_history": [
    {
      "commit": "45c9dfe3b26431e9282bbbb6078c7973d0db59c2",
      "review_id": 4730809436
    }
  ]
}

github-actions

Holistic Review

Motivation: Two latent, pre-existing HWIntrinsic miscompiles in gentree.cpp, found by inspection while auditing the file. (1) GetOperForHWIntrinsicId tested the isScalar out-param pointer (always non-null) instead of the dereferenced *isScalar, so a packed GT_SUB whose constant op1 has a zero low lane but is not all-zero (e.g. Vector128.Create(0,1,2,3) - x) was wrongly refined to GT_NEG, causing fgOptimizeHWIntrinsic's (-v1)+v2 => v2-v1 transform to drop the real constant. (2) A stray unconditional block in the min branch of gtNewSimdMinMaxNode overwrote needsFixup with IsVectorZero() for all four cases, making the preceding if/else dead and dropping the required AVX512 fixup for signed-zero constants, miscompiling Min/MinNumber against a signed-zero constant.

Approach: Minimal, surgical fixes. The first dereferences the flag (*isScalar) so the negate refinement applies only to scalar ops. The second removes the stray block so the min branch mirrors the already-correct max branch: isNumber uses IsFloatPositiveZero/IsVectorZero, otherwise IsFloatNegativeZero/IsVectorNegativeZero. Two focused regression tests are added under JitBlue/Runtime_130830 and JitBlue/Runtime_130831 and wired into Regression_ro_2.csproj so they build and run in CI.

Summary: LGTM. Both changes are correct and precisely targeted. I verified the corrected min branch is a faithful inverse mirror of the max branch's fixup logic, and that *isScalar is the intended guard. The regression tests are well-constructed: Runtime_130830 asserts per-lane results that would collapse to <90,90,90,90> under the old bug, and Runtime_130831 correctly compares signed-zero results bitwise (via BitConverter), since Double/Single.Equals treat -0.0/+0.0 as equal and would otherwise mask the defect. The added <Compile> entries are necessary because sibling tests in this directory are individually listed in the merged csproj. No actionable findings.

Note

This review was generated by this repository's Holistic Review agentic workflow to complement the built-in Copilot review.

Generated by Holistic Review · 41.2 AIC · ⌖ 10.4 AIC · ⊞ 10K

Mono's Min/MinNumber lowering doesn't preserve the sign of zero, tracked by dotnet#131130. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot

Copilot's findings

Files reviewed: 4/4 changed files
Comments generated: 0 new

tannergooding · 2026-07-21T02:16:51Z

@EgorBo, this one needs re-sign off because I had to skip a test on Mono LLVM due to diverged behavior. The fix in Mono is a little non-trivial and so it's better to handle as a follow-up than block the RyuJIT correctness fix.

… zero (#131172) Fixes #131130. `double.MinNumber`/`MaxNumber` (and the `float`/`Half` variants) implement IEEE 754-2019 `minimumNumber`/`maximumNumber`, which are NaN-suppressing **and** treat `-0` as less than `+0`. The Mono LLVM scalar lowering used `llvm.minnum`/`maxnum` (IEEE 754-2008 `minNum`/`maxNum`), which leave the sign of zero unspecified -- e.g. `minnum(+0, -0)` returns `+0` on x86, so `double.MinNumber(+0.0, -0.0)` returned `+0.0` instead of `-0.0` on the LLVM AOT configuration. Lower to `llvm.minimumnum`/`maximumnum` instead (available since LLVM 20; Mono is on LLVM 23). These are the exact 2019 `minimumNumber`/`maximumNumber` operations -- NaN-suppressing and sign-of-zero aware -- and on AArch64 still lower to a single `fminnm`/`fmaxnm`. `Min`/`Max` already use `llvm.minimum`/`maximum` and are correct, so they are unchanged. I confirmed the divergence and the fix by JIT-executing each intrinsic on x86-64: `minnum(+0, -0)` returns `+0` (wrong), while `minimumnum(+0, -0)` returns `-0` (correct) and still suppresses NaN. ---------- Un-skips the `Runtime_130831` regression test (added in #130832) on Mono, which covers the `Min`/`MinNumber` signed-zero cases directly and runs on the `AllSubsets_Mono_LLVMAot_RuntimeTests` leg. > [!NOTE] > This change was authored with the assistance of GitHub Copilot. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

tannergooding and others added 4 commits July 15, 2026 17:58

Add regression test for GetOperForHWIntrinsicId isScalar fix

b30845a

Covers packed integer `(cns - v1) + v2` where the constant's low lane is zero, which was previously miscompiled by dropping the constant. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Add regression test for gtNewSimdMinMaxNode needsFixup fix

ac4ba64

Covers Min/MinNumber with a signed-zero constant, the finite opposite-signed-zero case that Runtime_98068 never exercised. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot AI review requested due to automatic review settings July 16, 2026 01:46

github-actions Bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Jul 16, 2026

dotnet-policy-service Bot assigned tannergooding Jul 16, 2026

Copilot started reviewing on behalf of tannergooding July 16, 2026 01:47 View session

Copilot AI reviewed Jul 16, 2026

View reviewed changes

Comment thread src/tests/JIT/Regression/JitBlue/Runtime_130831/Runtime_130831.cs

Compare signed-zero results bitwise in Runtime_130831

2ad2d7e

Double/Single.Equals treat -0.0 and +0.0 as equal, so the original Assert.Equal assertions did not observe a wrong-signed-zero result. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot AI review requested due to automatic review settings July 16, 2026 02:08

Copilot started reviewing on behalf of tannergooding July 16, 2026 02:08 View session

Copilot AI reviewed Jul 16, 2026

View reviewed changes

Comment thread src/tests/JIT/Regression/JitBlue/Runtime_130830/Runtime_130830.cs

Comment thread src/tests/JIT/Regression/JitBlue/Runtime_130831/Runtime_130831.cs

Reference new JitBlue tests from Regression_ro_2.csproj

45c9dfe

The merged JitBlue tests need an explicit Compile entry to be built and run in CI; without it Runtime_130830 and Runtime_130831 were not exercised. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot AI review requested due to automatic review settings July 16, 2026 02:29

Copilot started reviewing on behalf of tannergooding July 16, 2026 02:29 View session

Copilot AI reviewed Jul 16, 2026

View reviewed changes

tannergooding mentioned this pull request Jul 16, 2026

Wire up orphaned Runtime_129288 regression test #130836

Merged

This was referenced Jul 16, 2026

The Operation will be canceled. The next steps may not contain expected logs. dotnet/dnceng#3008

Open

Flaky timeout in CreateFileChangeToken_RootDeletedAndRecreated_TokenFiresWhenFileCreated #129691

Closed

tannergooding requested a review from EgorBo July 18, 2026 16:44

build-analysis Bot mentioned this pull request Jul 18, 2026

Unable to pull image from mcr.microsoft.com #117164

Open

github-actions Bot reviewed Jul 19, 2026

View reviewed changes

EgorBo reviewed Jul 20, 2026

View reviewed changes

Comment thread src/coreclr/jit/gentree.cpp

EgorBo approved these changes Jul 20, 2026

View reviewed changes

tannergooding enabled auto-merge (squash) July 20, 2026 23:28

tannergooding mentioned this pull request Jul 21, 2026

Mono: Min/MinNumber lowering does not preserve the sign of zero #131130

Closed

Skip Runtime_130831 on Mono for signed-zero Min divergence

683376a

Mono's Min/MinNumber lowering doesn't preserve the sign of zero, tracked by dotnet#131130. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot AI review requested due to automatic review settings July 21, 2026 02:11

Copilot started reviewing on behalf of tannergooding July 21, 2026 02:11 View session

Copilot AI reviewed Jul 21, 2026

View reviewed changes

build-analysis Bot mentioned this pull request Jul 21, 2026

slow macOS - "##[error]The job running on agent Azure Pipelines 9 ran longer than the maximum time of 60 minutes." dotnet/dnceng#1883

Open

3 tasks

JulieLeeMSFT added the Priority:1 Work that is critical for the release, but we could probably ship without label Jul 21, 2026

JulieLeeMSFT approved these changes Jul 21, 2026

View reviewed changes

tannergooding merged commit 231d8f9 into dotnet:main Jul 21, 2026
149 checks passed

tannergooding deleted the gentree-correctness-fixes branch July 21, 2026 21:51

tannergooding mentioned this pull request Jul 21, 2026

Fix Mono LLVM lowering of MinNumber/MaxNumber to preserve the sign of zero #131172

Merged

dotnet-maestro Bot mentioned this pull request Jul 22, 2026

[main] Source code updates from dotnet/runtime dotnet/dotnet#7872

Merged

dotnet-maestro Bot mentioned this pull request Jul 23, 2026

[release/11.0.1xx-preview7] Source code updates from dotnet/runtime dotnet/dotnet#7923

Merged

dotnet-milestone-bot Bot modified the milestones: 11.0-rc1, 11.0-preview7 Jul 24, 2026

Uh oh!

Conversation

tannergooding commented Jul 16, 2026

Uh oh!

azure-pipelines Bot commented Jul 16, 2026

Uh oh!

dotnet-policy-service Bot commented Jul 16, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Copilot's findings

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot's findings

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot's findings

Uh oh!

tannergooding commented Jul 16, 2026

Uh oh!

github-actions Bot commented Jul 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot left a comment

Choose a reason for hiding this comment

Holistic Review

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot's findings

Uh oh!

tannergooding commented Jul 21, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

github-actions Bot commented Jul 19, 2026 •

edited

Loading